-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new kernel operations ELM_MAT, ASS_MAT #3551
Conversation
InstallMethod( \[\], "for GF2 matrix", | ||
[ IsGF2MatrixRep, | ||
IsPosInt, IsPosInt ], | ||
MAT_ELM_GF2MAT ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now redundant as we already do InstallMethod(MatElm, ..., MAT_ELM_GF2MAT);
later in the file.
[ IsGF2MatrixRep, | ||
IsPosInt, IsPosInt, | ||
IsObject ], | ||
SET_MAT_ELM_GF2MAT ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now redundant as we already do InstallMethod(SetMatElm, ..., SET_MAT_ELM_GF2MAT);
later in the file.
lib/mat8bit.gi
Outdated
@@ -88,7 +88,7 @@ InstallOtherMethod( \[\], "For a compressed MatFFE", | |||
#M <mat> [ <pos1>, <pos2> ] | |||
## | |||
|
|||
InstallMethod( \[\], "For a compressed MatFFE", | |||
InstallMethod( \[\,\], "For a compressed MatFFE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could of course also use MatElm
here or ELM_MAT
... shrug
[ IsPlistMatrixRep and IsMutable, IsPosInt, IsPosInt, IsObject ], | ||
function( m, row, col, ob ) | ||
m![ROWSPOS][row]![ELSPOS][col] := ob; | ||
end ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant, as we already (resp.: still) install identical methods for MatElm
and SetMatElm
above
5b31819
to
62b7426
Compare
One question to ask here is: What about ISB_MAT and UNB_MAT? Should they also become kernel operations, with aliases Thing is, I am not convinced these operations are useful at all. I wouldn't include them in the MatrixObj specification at all. I am in fact tempted to deprecate the syntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
I have tried MethodsOperation( \[\], 3 )
etc., and found that also the packages in GAPJulia
are affected.
(And the MatFE
vs MatFFE
typos could be corrected.)
@ThomasBreuer re GAPJulia: ah good -- I didn't check that. I did, however, check |
The kernel functions with the same name now delegate to these operations instead of ELM_LIST, ASS_LIST. Also turned MatElm, SetMatElm into synonyms for ELM_MAT, ASS_MAT. Code which uses MatrixObj for a long time and thus installed methods for MatElm, SetMatElm will thus work unchanged. Code which adapted to the recommendation in GAP 4.10 and thus instead installed methods for ELM_LIST, ASS_LIST with 3 resp. 4 arguments now should be adapted to (again) install methods for MatElm, SetMatElm for maximal compatibility. To ensure this code works after this change, though, we provide low ranked fallback methods in ELM_MAT, ASS_MAT which delegate to ELM_LIST, ASS_LIST. At the same time, we remove the low-ranged delegations from ELM_LIST, ASS_LIST to MatElm, SetMatElm to ensure we don't end up delegating back and forth endlessly.
... and not "For" -- this matches the style in the rest of the library.
875ba29
to
ea1a88c
Compare
Codecov Report
@@ Coverage Diff @@
## master #3551 +/- ##
=========================================
Coverage ? 84.19%
=========================================
Files ? 695
Lines ? 345053
Branches ? 0
=========================================
Hits ? 290512
Misses ? 54541
Partials ? 0
|
This continues the work begun in PR #3550.
The kernel functions with the same name now delegate to these operations instead of
ELM_LIST
,ASS_LIST
.Also turned
MatElm
,SetMatElm
into synonyms forELM_MAT
,ASS_MAT
.Code which already used MatrixObj for a long time and thus installed methods for
MatElm
,SetMatElm
will thus work unchanged. Code which adapted to the recommendation in GAP 4.10 and thus instead installed methods forELM_LIST
,ASS_LIST
with 3 resp. 4 arguments now should be adapted to (again) install methods forMatElm
,SetMatElm
for maximal compatibility. To ensure this codeworks after this change, though, we provide low ranked fallback methods in
ELM_MAT
,ASS_MAT
which delegate toELM_LIST
,ASS_LIST
. At the same time, we remove the low-ranked delegations fromELM_LIST
,ASS_LIST
toMatElm
,SetMatElm
to ensure we don't end up delegating back and forth endlessly.As far as I could tell, the only package which is (mildly) impacted by this is
cvec
; I'll shortly release an update of that which addresses all those concerns, though, and which should work seamlessly with and without this PR. Of course there might be further code out there which is not in deposited packages, and which I thus can't test, but I am confident it'll be OK, too -- though of course I can't be sure without being able to look at that code.